Skip to content

fix(har): do not report zero sizes for bodies that were never read - #42371

Closed
Ayaan Gazali (ayaangazali) wants to merge 1 commit into
microsoft:mainfrom
ayaangazali:fix-42370
Closed

fix(har): do not report zero sizes for bodies that were never read#42371
Ayaan Gazali (ayaangazali) wants to merge 1 commit into
microsoft:mainfrom
ayaangazali:fix-42370

Conversation

@ayaangazali

Copy link
Copy Markdown

Rationale

A redirect or basic-auth retry hop made through APIRequestContext is destroyed before its response
body is read (server/fetch.ts:496 and :507), so event.body is undefined when the tracer sees
it. The tracer turned that into a concrete zero:

this._storeResponseContent(event.body, content, 'other');   // sets content.size = 0 when undefined
if (!this._options.omitSizes)
  harEntry.response.bodySize = event.body?.length ?? 0;

The resulting entry contradicts itself. Recording a 302 that carries a 40 byte body produces:

HAR /redirect-me  status=302  bodySize=0  contentSize=0  contentLengthHeader=40

Content-Length: 40 sits in the entry's own headers next to bodySize: 0, which asserts the server
sent nothing. HAR 1.2 uses -1 for a size that is not available, and both fields already start at
-1.

The browser path in this same file already does the right thing: it only calls
_storeResponseContent inside response.internalBody().then(buffer => ...), so when the body cannot
be read the -1 survives. The two paths disagreed about the identical "body never captured" case.

Recording the sizes only when a body was actually captured makes the undefined-buffer branch of
_storeResponseContent unreachable, so that branch is deleted and the parameter stops being
optional. Net effect is fewer lines than before. An empty body is still recorded as 0, since an
empty Buffer is truthy and genuinely means zero bytes.

Test

should include redirects from API request already existed but only asserted the URLs and that
timings is defined, so nothing pinned the sizes. Two assertions go there rather than into a new
test. On current main they fail with Expected: -1, Received: 0.

Green: har.spec.ts 64 passed, global-fetch.spec.ts + browsercontext-fetch.spec.ts 192 passed,
flint clean.

Fixes #42370


the judgement call I am least sure of is whether anyone depends on the current 0, given how long
it has been that way. I checked it against the HAR 1.2 wording and against your own browser-side
handling rather than going on instinct, but you will know the compatibility risk better than I do.
freshman in college, happy to be told this is not worth changing :)

A redirect or basic-auth retry hop made through APIRequestContext is
destroyed before its response body is read, so the body never reaches
the tracer. It still recorded bodySize 0 and content.size 0, which
claims the server sent nothing.

The result contradicts itself: an entry can carry Content-Length 40 in
its own headers and bodySize 0 next to it. HAR 1.2 uses -1 for a size
that is not available, which is what these entries start as, and what
the browser path already leaves them as when the body cannot be read.

Only record the sizes when a body was actually captured. That makes the
undefined-buffer branch of _storeResponseContent unreachable, so it goes
away and the parameter stops being optional.
Copilot AI lite review requested due to automatic review settings August 23, 2026 17:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions

Copy link
Copy Markdown
Contributor

Test results for "tests 1"

4 flaky ⚠️ [chromium-library] › library/video.spec.ts:736 › screencast › should work with video+trace `@realtime-time-library-chromium-linux`
⚠️ [chromium-library] › library/chromium/chromium.spec.ts:213 › should intercept service worker requests (main and within) `@chromium-ubuntu-22.04-node20`
⚠️ [chromium-library] › library/trace-viewer-scrub.spec.ts:177 › should drag scrubber to select action `@chromium-ubuntu-22.04-node20`
⚠️ [firefox-page] › page/page-emulate-media.spec.ts:144 › should keep reduced motion and color emulation after reload `@firefox-ubuntu-22.04-node20`

51370 passed, 1238 skipped


Merge workflow run.

@github-actions

Copy link
Copy Markdown
Contributor

Test results for "MCP"

1 failed
❌ [firefox] › mcp/annotate.spec.ts:446 › should switch screencast to -s session on show --annotate @mcp-windows-latest-firefox

8141 passed, 1361 skipped


Merge workflow run.

@dcrousso

Copy link
Copy Markdown
Contributor

Ayaan Gazali (@ayaangazali) you need to respond to #42371 (comment) before we can proceed

@ayaangazali

Copy link
Copy Markdown
Author

Understood, and sorry for the wait. The CLA is not accepted yet. It needs the account owner to post the acceptance, and I have flagged it again on my side.

I will not say it is done until I can point at a passing license/cla check, since I got that wrong once already on #42342 and would rather not waste your time twice.

@dgozman

Copy link
Copy Markdown
Collaborator

Friendly ping on the CLA. If you don't accept the CLA, we won't be able to merge the PR.

@ayaangazali

Copy link
Copy Markdown
Author

Thanks for the ping, and sorry it is still outstanding. The CLA has to be accepted by the account owner and I have escalated it again on my side today.

I am not going to claim it is done until there is a passing license/cla check to point at, having got that wrong once already. If it is easier for you to just take the fix, please do, the change is two files and the issue has the full repro.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: HAR records bodySize 0 for redirect hops whose body was never read

4 participants